-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[AssetMapper] Deleting duplicated sentence #19453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Not sure about this change. It's true that it's a bit deep internal explanation, but AssetMapper is still too new ... and it's confusing to see those empty and "fake" imports that AssetMapper does to make the CSS files work. I'd say that we should keep the internal details ... maybe with some reword if we want. |
The important part is explained right above this box:
My point: When importing a CSS file into a JavaScript file, what else could possibly happen (besides creating those mentioned |
Let me show you a real example. It's from live.symfony.com which was upgraded to AssetMapper the other day: {
"imports": {
"app": "/assets/app-8ec4b11d9d04ad28fc3acd8a4aa42add.js",
"/assets/styles/app.css": "data:application/javascript,",
// ...
"/assets/@symfony/ux-live-component/live.min.css": "data:application/javascript,document.head.appendChild%28Object.assign%28document.createElement%28%22link%22%29%2C%7Brel%3A%22stylesheet%22%2Chref%3A%22%2Fassets%2F%40symfony%2Fux-live-component%2Flive.min-5108f988fb2a3dbb292d6feebc9db7e8.css%22%7D%29%29",
// ...
}
} See the "ugly" and empty |
|
frontend/asset_mapper.rst
Outdated
work by adding an importmap entry for each CSS file that is valid, but | ||
does nothing. AssetMapper adds a ``link`` tag for each CSS file, but when | ||
the JavaScript executes the ``import`` statement, nothing additional happens. | ||
JavaScript modules. AssetMapper makes this work by adding an importmap entry for each CSS file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original sentence can be improved, but this one feels the CSS entries are "normal" import map ones, when they are not.
Maybe add this precision in your rewording ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure what you mean - please take a look now.
c844fc1
to
035a424
Compare
Thomas, I've merged this PR .. but reverted most of its contents. I agree with Simon and I still think that it's important to explain the low level details of these CSS imports/entrypoints because they look very strange on the template the first time you see them. In the future, when people are more used to all these AssetMapper features, we can reword to remove these internal details. Thanks for understanding. |
Page: https://symfony.com/doc/6.4/frontend/asset_mapper.html#handling-css This is indeed better than symfony#19453 was; and much better than before. 1. "valid" in which sense? Valid importmap syntax?
This PR was merged into the 6.4 branch. Discussion ---------- [AssetMapper] Minor Page: https://symfony.com/doc/6.4/frontend/asset_mapper.html#handling-css This is indeed better than #19453 was; and much better than before. 1. "valid" in which sense? Valid importmap syntax? 2. What about something like: > These special entries are valid importmap syntax, but ignored by JavaScript. Then the last sentence could be shortened. Commits ------- ea212df [AssetMapper] Minor
Page: https://symfony.com/doc/6.4/frontend/asset_mapper.html#handling-css
Besides "nothing additional happens" didn't make sense to me.